ci: add custom source-ref desktop builds#96
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the CI/CD pipeline by introducing a "custom" build mode. This new mode allows developers to trigger desktop builds from any specified Git reference, such as a branch or tag, and ensures these builds are pinned to a precise commit SHA. It also establishes a separate prerelease tagging scheme for these custom builds, preventing them from interfering with or being promoted as standard nightly or release builds. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a custom build mode to the CI workflow, allowing builds from an explicit source git ref. The changes are well-structured, adding the necessary logic to handle this new mode, including resolving the ref to a pinned commit SHA and generating unique prerelease tags. The new functionality is also accompanied by corresponding tests, which is great. I have one suggestion to improve code clarity and reduce duplication in the main shell script.
scripts/ci/resolve-build-context.sh
Outdated
| if [ "${requested_build_mode}" = "custom" ]; then | ||
| echo "::error::${GITHUB_EVENT_NAME} runs do not support build_mode=custom." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This check for build_mode=custom is duplicated in the schedule case (lines 249-252). To make the script more DRY, consider consolidating this validation. You could add a single check before the case "${GITHUB_EVENT_NAME}" statement (around line 224) to ensure custom mode is only used with workflow_dispatch events.
For example:
if [ "${requested_build_mode}" = "custom" ] && [ "${GITHUB_EVENT_NAME}" != "workflow_dispatch" ]; then
echo "::error::build_mode=custom is only supported for workflow_dispatch events, not for '${GITHUB_EVENT_NAME}'." >&2
exit 1
fi
case "${GITHUB_EVENT_NAME}" in
# ...This would allow you to remove the custom mode checks from both the schedule and * branches of the case statement, centralizing the validation logic.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The Cargo.lock update in
syncDesktopVersionFilesrelies on a very specific[[package]]block layout (name and version adjacent with only whitespace), which may break if Cargo changes the lockfile formatting or inserts additional fields; consider using a more robust parser or a looser regex that tolerates extra lines betweennameandversionfor the target package.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Cargo.lock update in `syncDesktopVersionFiles` relies on a very specific `[[package]]` block layout (name and version adjacent with only whitespace), which may break if Cargo changes the lockfile formatting or inserts additional fields; consider using a more robust parser or a looser regex that tolerates extra lines between `name` and `version` for the target package.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
updateCargoLockPackageVersionlogic relies on fairly strict regex patterns for[[package]]blocks andname/versionlines; consider relaxing these (e.g., allow trailing spaces or comments) or using a Cargo.lock/TOML parser to avoid brittle failures if the file format changes slightly. - The fake git implementation for
rev-parseonly supportsHEAD; if future changes callrev-parsewith other refs, this will start failing tests, so it may be worth generalizing this stub now (e.g., accept arbitrary refs and always return the test SHA).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `updateCargoLockPackageVersion` logic relies on fairly strict regex patterns for `[[package]]` blocks and `name/version` lines; consider relaxing these (e.g., allow trailing spaces or comments) or using a Cargo.lock/TOML parser to avoid brittle failures if the file format changes slightly.
- The fake git implementation for `rev-parse` only supports `HEAD`; if future changes call `rev-parse` with other refs, this will start failing tests, so it may be worth generalizing this stub now (e.g., accept arbitrary refs and always return the test SHA).Hi @zouyonghe! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The Cargo.lock update logic hardcodes the package name "astrbot-desktop-tauri" in both
updateCargoLockPackageVersionand the tests; consider extracting this into a shared constant to avoid divergence if the crate name ever changes. - In
fake-git.sh, the newrev-parsehandler only supportsHEADand otherwise errors; you may want to either assert the expected ref in the calling tests or broaden the handler to handle arbitrary refs to avoid brittle failures if the script is reused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Cargo.lock update logic hardcodes the package name "astrbot-desktop-tauri" in both `updateCargoLockPackageVersion` and the tests; consider extracting this into a shared constant to avoid divergence if the crate name ever changes.
- In `fake-git.sh`, the new `rev-parse` handler only supports `HEAD` and otherwise errors; you may want to either assert the expected ref in the calling tests or broaden the handler to handle arbitrary refs to avoid brittle failures if the script is reused.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
syncDesktopVersionFiles,updateCargoLockPackageVersionthrows if the target crate/package/version lines are not found, which will fail the whole version-sync step wheneverCargo.lockexists but doesn’t containastrbot-desktop-tauri(e.g., workspace or partially checked-in lockfiles); consider handling this case more leniently (logging and skipping the update) so the job doesn’t hard-fail in those scenarios.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `syncDesktopVersionFiles`, `updateCargoLockPackageVersion` throws if the target crate/package/version lines are not found, which will fail the whole version-sync step whenever `Cargo.lock` exists but doesn’t contain `astrbot-desktop-tauri` (e.g., workspace or partially checked-in lockfiles); consider handling this case more leniently (logging and skipping the update) so the job doesn’t hard-fail in those scenarios.
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="108-113" />
<code_context>
+ blockStartIndex = index;
+ }
+
+ if (!foundTargetPackage || !foundTargetVersion) {
+ throw new Error(`Cannot update Cargo.lock package version for ${packageName}`);
+ }
</code_context>
<issue_to_address>
**suggestion:** Differentiate error causes when updating Cargo.lock to aid debugging.
Since you already track `foundTargetPackage` and `foundTargetVersion`, consider throwing distinct errors for each condition. That way callers can tell whether the package is missing from Cargo.lock or the expected version line/layout is missing, which will make failures easier to diagnose.
```suggestion
blockStartIndex = index;
}
if (!foundTargetPackage) {
throw new Error(`Cannot update Cargo.lock: package "${packageName}" not found`);
}
if (!foundTargetVersion) {
throw new Error(
`Cannot update Cargo.lock: version entry for package "${packageName}" not found or has an unexpected layout`
);
}
```
</issue_to_address>
### Comment 2
<location path="scripts/prepare-resources/version-sync.mjs" line_range="54" />
<code_context>
+const escapeRegExp = (value) => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
+const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
+ const packageHeaderPattern = /^\s*\[\[package\]\]\s*(?:#.*)?$/;
+ const packageNamePattern = new RegExp(
</code_context>
<issue_to_address>
**issue (complexity):** Consider rewriting the Cargo.lock update logic as a single-pass state machine instead of using a nested helper and block indices to track package sections.
The nested `updateBlock` plus `blockStartIndex` scan can be flattened into a single pass that keeps the same behavior (comments preserved, extra fields allowed, only first matching package/version updated, error if name or version missing), while reducing state and indirection.
You can keep the existing regexes and CRLF handling, but replace the block logic with a simple streaming state machine:
```ts
const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
const packageHeaderPattern = /^\s*\[\[package\]\]\s*(?:#.*)?$/;
const packageNamePattern = new RegExp(
`^\\s*name\\s*=\\s*"${escapeRegExp(packageName)}"\\s*(?:#.*)?$`,
);
const packageVersionPattern = /^(\s*version\s*=\s*")[^"]+(")(\s*(?:#.*)?)$/;
const lines = cargoLock.split(/\r?\n/);
let inPackageBlock = false;
let inTargetPackage = false;
let foundTargetPackage = false;
let foundTargetVersion = false;
for (let i = 0; i < lines.length; i += 1) {
const line = lines[i];
// Detect start of a new [[package]] block
if (packageHeaderPattern.test(line)) {
inPackageBlock = true;
inTargetPackage = false;
continue;
}
if (!inPackageBlock) {
continue;
}
if (!inTargetPackage) {
if (packageNamePattern.test(line)) {
inTargetPackage = true;
foundTargetPackage = true;
}
continue;
}
// We are inside the target package block
if (packageHeaderPattern.test(line)) {
// Safety: new header before version -> stop looking in this block
inPackageBlock = true;
inTargetPackage = false;
continue;
}
if (packageVersionPattern.test(line)) {
lines[i] = line.replace(packageVersionPattern, `$1${version}$2$3`);
foundTargetVersion = true;
break;
}
}
if (!foundTargetPackage || !foundTargetVersion) {
throw new Error(`Cannot update Cargo.lock package version for ${packageName}`);
}
return lines.join(cargoLock.includes('\r\n') ? '\r\n' : '\n');
};
```
Key changes:
- Single loop over `lines` instead of `blockStartIndex` + `updateBlock`.
- State reduced to `inPackageBlock` and `inTargetPackage` (plus the two boolean outcome flags).
- Header detection is no longer duplicated in the inner closure.
- No closure mutating outer-scope variables, making control flow easier to follow while preserving all existing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="62" />
<code_context>
+ return error;
+};
+
+const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
+ const packageHeaderPattern = /^\s*\[\[package\]\]\s*(?:#.*)?$/;
+ const packageNamePattern = new RegExp(
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the Cargo.lock update logic to use block-based parsing, simpler line handling, and minimal error types to make the code easier to follow while preserving behavior.
You can reduce the cognitive load of the new logic without losing any behavior by:
1. **Making `updateCargoLockPackageVersion` block-based instead of flag-based**
2. **Dropping the extra regex complexity for the version line**
3. **Trimming the error plumbing to just what’s needed**
### 1. Refactor to block-based processing
Instead of maintaining `inPackageBlock` / `inTargetPackage` / `foundTargetVersion` flags, you can work in small “package block” slices.
Example sketch (focus on structure, not exact edge cases):
```js
const PACKAGE_HEADER = /^\s*\[\[package\]\]/;
const findPackageBlocks = (lines) => {
const blocks = [];
let start = null;
for (let i = 0; i < lines.length; i++) {
if (PACKAGE_HEADER.test(lines[i])) {
if (start !== null) {
blocks.push({ start, end: i });
}
start = i;
}
}
if (start !== null) {
blocks.push({ start, end: lines.length });
}
return blocks;
};
const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
const lines = cargoLock.split(/\r?\n/);
const blocks = findPackageBlocks(lines);
const nameLinePattern = new RegExp(
`^\\s*name\\s*=\\s*"${escapeRegExp(packageName)}"\\s*(?:#.*)?$`
);
let foundPackage = false;
let updated = false;
for (const { start, end } of blocks) {
const blockLines = lines.slice(start, end);
if (!blockLines.some((line) => nameLinePattern.test(line))) continue;
foundPackage = true;
for (let i = start; i < end; i++) {
const line = lines[i];
const trimmed = line.trimStart();
if (!trimmed.startsWith('version')) continue;
lines[i] = updateVersionLine(line, version);
updated = true;
break;
}
break; // only one matching package expected
}
if (!foundPackage) {
throw new CargoLockPackageNotFoundError(packageName);
}
if (!updated) {
throw new Error(
`Cannot update Cargo.lock: version entry for package "${packageName}" not found or has an unexpected layout`
);
}
return lines.join(cargoLock.includes('\r\n') ? '\r\n' : '\n');
};
```
This removes the implicit state machine and makes the control flow “find block → if name matches → update version line”.
### 2. Simplify version-line handling (no regex replacement groups)
You can replace `packageVersionPattern` + `.replace()` with a small helper that preserves spacing and comments:
```js
const updateVersionLine = (line, version) => {
const [beforeComment, comment = ''] = line.split('#', 2);
const [left, rightRaw = ''] = beforeComment.split('=', 2);
if (!rightRaw) return line; // unexpected layout, let caller decide
const rhsPrefix = rightRaw.match(/^\s*["']?/u)?.[0] ?? '';
const quote = rhsPrefix.includes('"') || rhsPrefix.includes("'") ? rhsPrefix.trim().slice(0, 1) : '"';
const newBeforeComment = `${left.trimEnd()} = ${quote}${version}${quote}`;
return comment ? `${newBeforeComment} #${comment}` : newBeforeComment;
};
```
Then `updateCargoLockPackageVersion` just calls `updateVersionLine(line, version)` instead of the capturing-group regex replacement.
### 3. Simplify error plumbing
You only treat “package not found” specially, so you can:
- Use a small local error subclass, and
- Drop the generic factory and the unused `CARGO_LOCK_VERSION_NOT_FOUND`.
```js
class CargoLockPackageNotFoundError extends Error {
constructor(packageName) {
super(`Cannot update Cargo.lock: package "${packageName}" not found`);
this.code = 'cargo-lock-package-not-found';
}
}
// usage in updateCargoLockPackageVersion:
if (!foundPackage) {
throw new CargoLockPackageNotFoundError(packageName);
}
```
Caller stays almost the same:
```js
try {
updatedCargoLock = updateCargoLockPackageVersion({ ... });
} catch (error) {
if (error?.code === 'cargo-lock-package-not-found') {
console.warn(`${cargoLockPath}: ${error.message}. Skipping Cargo.lock version sync.`);
} else {
throw error;
}
}
```
This keeps all current behavior but reduces:
- Boolean flag juggling and control-flow branches,
- Regex complexity,
- Extra abstraction around error creation/codes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="134-137" />
<code_context>
+ continue;
+ }
+
+ for (let index = packageNameLineIndex + 1; index < end; index += 1) {
+ if (!lines[index].trimStart().startsWith('version')) {
+ continue;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tighten version-line detection to avoid matching unintended keys.
This condition will also match any key starting with `version` (e.g. `versioned_dep = ...`), which could cause incorrect edits if similar keys are added in `Cargo.lock`. Consider making the check stricter by requiring an `=` and a word boundary, e.g. testing the line against `/^\s*version\s*=/` instead of using `startsWith`.
```suggestion
for (let index = packageNameLineIndex + 1; index < end; index += 1) {
if (!/^\s*version\s*=/.test(lines[index])) {
continue;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
updateVersionLine, switching the quote style based on the presence of a single quote in the existing value risks generating single-quoted strings, which are not valid in Cargo.lock/TOML; it would be safer to always use double quotes and escape as needed while still preserving trailing whitespace and comments. - The custom build-mode validation logic for unsupported
GITHUB_EVENT_NAMEvalues (schedule, others) is spread across multiple case branches inresolve-build-context.sh; consider centralizing thebuild_mode=customeligibility checks to avoid divergence if new event types or modes are added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updateVersionLine`, switching the quote style based on the presence of a single quote in the existing value risks generating single-quoted strings, which are not valid in Cargo.lock/TOML; it would be safer to always use double quotes and escape as needed while still preserving trailing whitespace and comments.
- The custom build-mode validation logic for unsupported `GITHUB_EVENT_NAME` values (`schedule`, others) is spread across multiple case branches in `resolve-build-context.sh`; consider centralizing the `build_mode=custom` eligibility checks to avoid divergence if new event types or modes are added.
## Individual Comments
### Comment 1
<location path="scripts/prepare-resources/version-sync.mjs" line_range="63" />
<code_context>
+ }
+}
+
+const findCargoLockPackageBlocks = (lines) => {
+ const blocks = [];
+ let start = null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the Cargo.lock update logic into a single linear pass with a status flag instead of multiple helpers and custom error types.
You can keep the new behavior but reduce complexity by collapsing the “block parsing + line rewriting + custom error” machinery into a single linear pass with simpler error handling.
### 1. Replace block segmentation with a single‑pass state machine
Instead of `findCargoLockPackageBlocks` + nested loops, track state as you stream through the lines once. This removes `blocks`, `start/end`, and the extra helper.
```js
const updateCargoLockPackageVersion = ({ cargoLock, packageName, version }) => {
const lines = cargoLock.split(/\r?\n/);
const namePattern = new RegExp(
`^\\s*name\\s*=\\s*"${escapeRegExp(packageName)}"\\s*(?:#.*)?$`,
);
const newline = cargoLock.includes('\r\n') ? '\r\n' : '\n';
let inPackage = false;
let inTargetPackage = false;
let updated = false;
for (let i = 0; i < lines.length; i += 1) {
const line = lines[i];
if (CARGO_LOCK_PACKAGE_HEADER.test(line)) {
inPackage = true;
inTargetPackage = false;
continue;
}
// Any new [[...]] header ends the current package block
if (inPackage && /^\s*\[\[/.test(line)) {
inPackage = false;
inTargetPackage = false;
}
if (!inPackage) continue;
if (!inTargetPackage && namePattern.test(line)) {
inTargetPackage = true;
continue;
}
if (inTargetPackage && CARGO_LOCK_VERSION_LINE.test(line)) {
const updatedLine = updateVersionLine(line, version);
if (!updatedLine) {
throw new Error(
`Cannot update Cargo.lock: version entry for package "${packageName}" has an unexpected layout`,
);
}
lines[i] = updatedLine;
updated = true;
break;
}
}
return {
content: updated ? lines.join(newline) : cargoLock,
updated,
};
};
```
This preserves:
- `[[package]]` detection
- name matching
- version line rewriting (via your existing `updateVersionLine`)
- CRLF vs LF handling
…while eliminating `findCargoLockPackageBlocks` and the extra nested loops.
### 2. Simplify error signaling for the caller
Instead of a dedicated `CargoLockPackageNotFoundError`, let the helper return a status flag; the caller can decide whether “not updated” means “not found” or “unchanged”.
```js
if (existsSync(cargoLockPath)) {
const cargoLock = await readFile(cargoLockPath, 'utf8');
const { content: updatedCargoLock, updated } = updateCargoLockPackageVersion({
cargoLock,
packageName: DESKTOP_TAURI_CRATE_NAME,
version,
});
if (!updated) {
console.warn(
`${cargoLockPath}: package "${DESKTOP_TAURI_CRATE_NAME}" not found or version not updated. Skipping Cargo.lock version sync.`,
);
} else if (updatedCargoLock !== cargoLock) {
await writeFile(cargoLockPath, updatedCargoLock, 'utf8');
}
}
```
This keeps all existing behavior (including the warning) but:
- removes a custom error type and extra `try/catch` branch
- keeps `syncDesktopVersionFiles` more linear and closer in complexity to the other version‑update paths.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
updateVersionLine, usingright.includes("'")to choose the quote character can break valid Cargo.lock lines likeversion = "1.0.0 with user's patch"by switching to single quotes; consider preserving the original quote character (e.g., by detecting the first quote in the value) instead of inferring from content. - The new
syncDesktopVersionFilesCargo.lock tests repeat a lot of boilerplate setup for the temp project structure; factoring that into a small helper (e.g.,createTempDesktopProject({ cargoLockContents })) would make future scenarios easier to add and the tests more readable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updateVersionLine`, using `right.includes("'")` to choose the quote character can break valid Cargo.lock lines like `version = "1.0.0 with user's patch"` by switching to single quotes; consider preserving the original quote character (e.g., by detecting the first quote in the value) instead of inferring from content.
- The new `syncDesktopVersionFiles` Cargo.lock tests repeat a lot of boilerplate setup for the temp project structure; factoring that into a small helper (e.g., `createTempDesktopProject({ cargoLockContents })`) would make future scenarios easier to add and the tests more readable.Hi @zouyonghe! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.|
@sourcery-ai review |
Summary
custombuild mode for explicitsource_git_refdesktop buildsnightlyTesting
Summary by Sourcery
Introduce a custom desktop build mode that pins explicit source refs to commit SHAs and ensure desktop version sync also updates Cargo.lock robustly.
New Features:
Enhancements:
Tests: